Skip to content

London | May-2025 | Ikenna Agulobi | Structuring and Testing Data sprint-3 #614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

ike-agu
Copy link

@ike-agu ike-agu commented Jun 26, 2025

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.
This PR includes my completed exercises for Sprint 3 in the Structuring and Testing Data module.

###The exercises included in this PR:###

1- key-implement
2- mandatory-rewrite
3- mandatory-practice
4- stretch-investigate

Questions

Ask any questions you have for your reviewer.

Hello @cjyuan - I wasn't able to complete the test cases for Card validator in Stretch-investigate. Given that it's not among mandatory exercises, can I come back to it when I have more time as I'm a bit behind?

@ike-agu ike-agu added the Needs Review Participant to add when requesting review label Jun 26, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Jul 4, 2025

Your PR is created against CYF's coursework/sprint-3 and not against CYF's main. Can you change the current PR so that it is compared to CYF's main?

@cjyuan cjyuan added the 👀 Review Git Changes requested to do with Git label Jul 4, 2025
@ike-agu ike-agu changed the base branch from to-be-deleted-coursework/sprint-3 to main July 4, 2025 10:48
@ike-agu
Copy link
Author

ike-agu commented Jul 4, 2025

Hi @cjyuan , I've updated my PR to point to CodeYourFuture:main as requested. Please let me know if anything else needs to be changed. Thank you!

@cjyuan
Copy link
Contributor

cjyuan commented Jul 4, 2025

This branch was created from branch "coursework/sprint-2" instead of from main. As a result, it includes changes made in "Sprint-2" folder.

Can you rebase your coursework/sprint-3 branch onto main?

The following instructions assume you had created a branch named B2 (coursework/sprint-3) from a branch named B1 (coursework/sprint-2) instead of from main, and you wanted to rebase B2 from B1 onto main.

Important:

  • You need to execute the commands within your cloned repository.
  • You may want to backup your files before trying these commands

1. Open Your Cloned Repository in VSCode and Start a Terminal in VSCode.

VSCode will start the terminal in the top-level folder of the current project.

2. Switch to the branch you want to rebase (B2)

git switch B2

Note:

  • You can check which branch is the current branch via the command git branch (to list all branches with current branch highlighted)

3. Rebase B2 from B1 onto main

git rebase --onto main B1 B2

For more details about this command, ask an AI tool or see
https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase#:~:text=interactive%20rebase%20display-,Advanced%20rebase%20application,-The%20command%20line

4. Update (and Overwrite) your files in the remote branch B2 (on Github)

While you are in branch B2 and you have verified that it has been successfully rebased, execute the following command to update the remote branch (on GitHub):
git push --force origin

ike-agu added 23 commits July 5, 2025 11:54
@ike-agu ike-agu force-pushed the coursework/sprint-3 branch from 4ad048f to 79fcbe9 Compare July 5, 2025 11:08
@ike-agu
Copy link
Author

ike-agu commented Jul 5, 2025

Hi @cjyuan , thank you for your feedback. I've now rebased the coursework/sprint-3 branch onto main as you requested, and I have removed the unintended changes from sprint-2 and I've also force-pushed the updated branch to GitHub.
Please let me know if there's anything else I need to adjust. Thanks!!

Comment on lines 17 to 26
const isValidPassword = require("./password-validator.js");
test("password has at least 5 characters", () => {
// Arrange
const password = "12345";
const password = "Abc1!";
// Act
const result = isValidPassword(password);
// Assert
expect(result).toEqual(true);
}
); No newline at end of file
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check only ensure the function can return true when the password is correct; it does not ensure the function can return false when a password does not meet the condition specified.

The same applies to the other tests in this script.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added test cases to check that my function returns false when password is too short, has no special character, has no number, no upper case and has no lower case as you've requested. Thanks!

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed 👀 Review Git Changes requested to do with Git Needs Review Participant to add when requesting review labels Jul 5, 2025
@ike-agu
Copy link
Author

ike-agu commented Jul 7, 2025

Hi @cjyuan , I've implemented all the changes you requested. Could you please take a look. Thank you 🙏

Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Well done.

// add your completed function from key-implement here
if (numerator >= denominator) return false;
if(denominator === 0){
return "Denominator cannot be zero"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return a string from a function that is expected to return a Boolean value can make consuming the return value inconvenient.

Consider the alternatives:

  • return false (to indicate anything divided by 0 is not a proper fraction)
  • throw an error

@@ -56,3 +56,34 @@ test("password must not be any previous password in the passwords array", ()=> {
const result = isValidPassword(password);
expect(result).toEqual(false);
});

test("fails when password is less than 5 characters", () => {
const password = "A1!";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also include a lowercase letter.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and review comments have been addressed and removed Reviewed Volunteer to add when completing a review labels Jul 7, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Jul 7, 2025

You forgot to change label to "Needs review". :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Volunteer to add when work is complete and review comments have been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants